Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge OBC package from Buildings to IBPSA #1946

Merged
merged 22 commits into from
Jan 6, 2025
Merged

Merge OBC package from Buildings to IBPSA #1946

merged 22 commits into from
Jan 6, 2025

Conversation

mwetter
Copy link
Contributor

@mwetter mwetter commented Dec 14, 2024

This closes #1944

It also moves the round function in the math package as it is used in various Elementary Blocks, and it moves some Composite Control blocks that are likely of general interest.

@mwetter mwetter marked this pull request as ready for review December 16, 2024 15:24
@mwetter mwetter requested a review from FWuellhorst December 16, 2024 16:33
Copy link
Contributor

@FWuellhorst FWuellhorst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mwetter I will look into the CDL package in the upcoming days, just noticed a minor naming issue outside of that package.

IBPSA/Utilities/Math/Functions/round.mo Outdated Show resolved Hide resolved
@FWuellhorst
Copy link
Contributor

FWuellhorst commented Dec 17, 2024

Aside from that comment, the documentation and naming looks ok.
However, you maybe should note in the wiki on naming that you chose a different naming approach for CDL package, as this is heavily oriented at the MSL package or maybe the standard.
Else, you have a lot of complete words and control specific terms in there, not listed as possible exceptions from your naming rules.

Otherwise, I only have two requests:

  • Add a central UsersGuide on what this package is and why one should use it. You can refer to existing doc elsewhere if you want.
  • Maybe state in that UsersGuide or model doc what the difference to the MSL blocks are, and if changing them is as easy as replacing the MSL block, e.g. and with the one in CDL. Looking at the models, I sometimes do not get why you needed to duplicate a lot of stuff from MSL. Avoid inheritance is one reason, but why add e.g. RealInput again? If switching is easy, do you have a script to automatically switch, if possible? Also, are all blocks exactly the same in terms of functionality, e.g. IBPSA.Controls.OBC.CDL.Logical.Timer? I directly see you added the passed option, but don't get why you did not just compose a new block out of the standard timer and a greater threshold.

@mwetter
Copy link
Contributor Author

mwetter commented Jan 3, 2025

@FWuellhorst : Thanks for the review. The two points above have been addressed. Can you please review and approve it all is fine.

@mwetter mwetter requested a review from FWuellhorst January 3, 2025 22:39
Copy link
Contributor

@FWuellhorst FWuellhorst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The UserGuide is clear and concise, and the models all look good.

@mwetter mwetter merged commit 8797a23 into master Jan 6, 2025
3 checks passed
@mwetter mwetter deleted the issue1944_cdl branch January 6, 2025 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Merge Buildings.Controls.OBC.CDL
2 participants